Skip to content

Conversation

@fm3
Copy link
Member

@fm3 fm3 commented Sep 16, 2025

  • Adds surface area segment stat, based on mesh triangle surfaces
  • Fixes fullMesh loading with ad-hoc meshes, also used in render_animation worker jobs

URL of deployed dev instance (used for testing):

Steps to test:

  • For dataset with segment index file, and also for volume annotation:
    • Context menu should now have explicitly triggered loading of segment statistics
    • Surface area should be included

TODOs:

  • Fix Infinity/NaN
  • Validate math/output value against meshlab
  • move to segment statistics route
  • also allow for adhoc/volume/proofreading?
  • Frontend
    • Add surface area segment statistic
    • Move segment statistic out of toplevel rightclick menu to save computation
    • Select correct combination of meshing parameters for the new route
  • Wait for Use datastore segment index when loading ad-hoc meshes #8922 to enable static layer ad-hoc without seedPosition
  • Re-Test proofreading ad-hoc, do we need to set editableMappingTracingId or is the layer doing this for us already?

Issues:


  • Added changelog entry (create a $PR_NUMBER.md file in unreleased_changes or use ./tools/create-changelog-entry.py)
  • Removed dev-only changes like prints and application.conf edits
  • Considered common edge cases
  • Needs datastore update after deployment

@fm3 fm3 self-assigned this Sep 16, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 16, 2025

📝 Walkthrough

Walkthrough

Adds mesh-based segment surface-area end-to-end: new mesh request types and helpers, STL surface-area computation, datastore and tracingstore endpoints and routes, frontend API/UI to request/display/export surface area, i18n update, and dataset-not-found propagation in WK controllers.

Changes

Cohort / File(s) Summary
WK controllers
app/controllers/WKRemoteDataStoreController.scala, app/controllers/WKRemoteTracingStoreController.scala
Propagate dataset-not-found to 404 with localized message on dataset lookups; add getDataSource(name,key,datasetId) endpoint to tracing-store controller.
Routing tables
conf/webknossos.latest.routes, webknossos-datastore/conf/datastore.latest.routes, webknossos-tracingstore/conf/tracingstore.latest.routes
Reintroduce dataset read route; add dataset reserve/compose routes; expose tracingstore datasource route; add POST routes for segment surfaceArea (datastore and tracingstore).
i18n & changelog
conf/messages, unreleased_changes/8928.md
Add mesh.loadFull.failed message key; add changelog entry indicating segment surface-area display.
Frontend API & utils
frontend/javascripts/admin/rest_api.ts, frontend/javascripts/libs/format_utils.ts
Add SegmentStatisticsParametersMeshBased type and getSegmentSurfaceArea API call; trivial comment fix in format_utils.
Frontend viewer & UI
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts, frontend/javascripts/viewer/view/context_menu.tsx, frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx
Add getCurrentMappingName; extend context menu and modal to request, display, and export surface area alongside volume and bounding box; UI/CSV/table updates and formatting.
Datastore controller & helpers
webknossos-datastore/app/.../controllers/DataSourceController.scala, webknossos-datastore/app/.../helpers/SegmentStatistics.scala, webknossos-datastore/app/.../services/mesh/FullMeshHelper.scala
Inject DSFullMeshService; add mesh-based getSegmentSurfaceArea endpoint and SegmentStatisticsParametersMeshBased type/JSON formatter; add surfaceAreaFromStlBytes to compute area from STL bytes.
Tracingstore clients/services/controllers
webknossos-tracingstore/app/.../TSRemoteDatastoreClient.scala, webknossos-tracingstore/app/.../TSRemoteWebknossosClient.scala, webknossos-tracingstore/app/.../controllers/VolumeTracingController.scala, webknossos-tracingstore/app/.../tracings/volume/TSFullMeshService.scala
Remove voxel-size caching from TSRemoteDatastoreClient; add cached voxel-size lookup to TSRemoteWebknossosClient; add VolumeTracingController.getSegmentSurfaceArea endpoint; switch full-mesh voxel-size source to remote-webknossos client; minor mesh-load error-key update.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • MichaelBuessemeyer
  • philippotto

Poem

I nibble bytes and count each face,
Triangles hush and hold their place.
From STL folds I tally land,
Surface measured by rabbit's hand.
Hop—statistics ready on demand! 🐇

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The pull request introduces dataset CRUD endpoints and enhancements to remote data store and tracing store controllers that are not directly related to computing segment surface area or fixing fullMesh loading as specified by the linked issue. These additions of reserveUploadToPaths, compose routes, and generalized error-handling in data store controllers extend functionality beyond the stated objectives. Please remove or relocate the unrelated dataset and tracing store endpoint changes into separate pull requests or clarify how they serve as prerequisites for the surface area feature.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the primary addition of segment surface area computation and aligns directly with the main change implemented in the pull request. It is concise, clear, and uses specific phrasing that a teammate scanning history would understand.
Linked Issues Check ✅ Passed The pull request fully implements the objectives of issue #8610 by computing surface area from mesh triangle surfaces, exposing new endpoints for segment surface area, and integrating support throughout both backend and frontend for layers with precomputed mesh files. It also fixes fullMesh loading for ad-hoc meshes, enabling the necessary workflow for surface area stats.
Description Check ✅ Passed The description outlines the addition of segment surface area statistics, the fullMesh loading fixes, testing steps, and related TODOs, demonstrating clear relevance to the changeset. It provides context and guidance for verification without deviating into unrelated topics.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mesh-surface-area

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@fm3 fm3 marked this pull request as ready for review October 13, 2025 08:17
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (5)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)

78-114: Surface area calculation looks correct, consider adding input validation.

The algorithm correctly computes surface area from STL binary format:

  • Proper parsing of STL structure (80-byte header, 4-byte triangle count, 50 bytes per triangle)
  • Correct triangle area calculation using cross product magnitude divided by 2
  • Appropriate error handling with tryo

Consider adding input validation to make the algorithm more robust against malformed data:

 def surfaceAreaFromStlBytes(stlBytes: Array[Byte]): Box[Float] = tryo {
   val dataBuffer = ByteBuffer.wrap(stlBytes)
   dataBuffer.order(ByteOrder.LITTLE_ENDIAN)
+  if (stlBytes.length < 84) {
+    throw new IllegalArgumentException(s"STL data too short: ${stlBytes.length} bytes, minimum 84 bytes required")
+  }
   val numberOfTriangles = dataBuffer.getInt(80)
+  val expectedSize = 84 + numberOfTriangles * 50
+  if (stlBytes.length < expectedSize) {
+    throw new IllegalArgumentException(s"STL data incomplete: expected $expectedSize bytes, got ${stlBytes.length}")
+  }
   val normalOffset = 12
   var surfaceSumMutable = 0.0f
   val headerOffset = 84
   val bytesPerTriangle = 50
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)

951-960: Return mapping only when enabled to avoid using disabled mappings

Currently, a disabled mapping can be returned. Prefer honoring mappingStatus and return null/undefined when not ENABLED.

Apply this refinement:

 export function getCurrentMappingName(state: WebknossosState) {
   const visibleSegmentationLayer = getVisibleSegmentationLayer(state);
   const volumeTracing = getActiveSegmentationTracing(state);
   if (volumeTracing?.mappingName != null) return volumeTracing?.mappingName;
   const mappingInfo = getMappingInfo(
     state.temporaryConfiguration.activeMappingByLayer,
     visibleSegmentationLayer?.name,
   );
-  return mappingInfo.mappingName;
+  return mappingInfo.mappingStatus === MappingStatusEnum.ENABLED
+    ? mappingInfo.mappingName
+    : null;
 }
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)

58-58: Bound the voxel size cache to avoid unbounded growth

Consider adding a maxCapacity similar to annotationIdByTracingIdCache to prevent unbounded entries during long‑running servers.

-  private lazy val voxelSizeCache: AlfuCache[ObjectId, VoxelSize] = AlfuCache(timeToLive = 10 minutes)
+  private lazy val voxelSizeCache: AlfuCache[ObjectId, VoxelSize] =
+    AlfuCache(maxCapacity = 10000, timeToLive = 10 minutes)
frontend/javascripts/viewer/view/context_menu.tsx (2)

1666-1672: Harden refresh action with error handling

Persist may fail; keep the menu usable and inform users.

-  const handleRefreshSegmentStatistics = async () => {
-    await api.tracing.save();
-    setSegmentStatsTriggerDate(new Date());
-  };
+  const handleRefreshSegmentStatistics = async () => {
+    try {
+      await api.tracing.save();
+    } catch (e) {
+      Toast.error("Saving the annotation failed. Showing last known statistics.");
+    } finally {
+      setSegmentStatsTriggerDate(new Date());
+    }
+  };

1757-1758: Include mappingName in fetch dependencies

Prevents stale stats when the active mapping changes.

-    [contextMenuPosition, isSegmentIndexAvailable, clickedSegmentOrMeshId, segmentStatsTriggerDate],
+    [
+      contextMenuPosition,
+      isSegmentIndexAvailable,
+      clickedSegmentOrMeshId,
+      segmentStatsTriggerDate,
+      mappingName,
+    ],
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e95351c and 7d22add.

📒 Files selected for processing (20)
  • app/controllers/DatasetController.scala (1 hunks)
  • app/controllers/WKRemoteDataStoreController.scala (1 hunks)
  • app/controllers/WKRemoteTracingStoreController.scala (1 hunks)
  • conf/messages (1 hunks)
  • conf/webknossos.latest.routes (2 hunks)
  • frontend/javascripts/admin/rest_api.ts (1 hunks)
  • frontend/javascripts/libs/format_utils.ts (1 hunks)
  • frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1 hunks)
  • frontend/javascripts/viewer/view/context_menu.tsx (13 hunks)
  • frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (9 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DSMeshController.scala (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (2 hunks)
  • webknossos-datastore/conf/datastore.latest.routes (1 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (4 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (3 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1 hunks)
  • webknossos-tracingstore/conf/tracingstore.latest.routes (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
app/controllers/WKRemoteTracingStoreController.scala (5)
app/controllers/WKRemoteDataStoreController.scala (1)
  • getDataSource (260-270)
app/models/annotation/TracingStore.scala (1)
  • validateAccess (47-58)
app/models/dataset/Dataset.scala (1)
  • findOne (203-210)
util/src/main/scala/com/scalableminds/util/accesscontext/AccessContext.scala (1)
  • GlobalAccessContext (25-29)
app/models/dataset/DatasetService.scala (1)
  • dataSourceFor (390-399)
frontend/javascripts/admin/rest_api.ts (5)
frontend/javascripts/viewer/constants.ts (1)
  • Vector3 (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (2)
  • AdditionalCoordinate (90-95)
  • AdditionalCoordinate (97-112)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
  • getSegmentSurfaceArea (618-649)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (2)
  • getSegmentSurfaceArea (351-379)
  • data (215-234)
frontend/javascripts/admin/api/token.ts (1)
  • doWithToken (39-74)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)
  • voxelSizeForAnnotationWithCache (131-135)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (2)
frontend/javascripts/viewer/store.ts (1)
  • WebknossosState (575-595)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (2)
  • getVisibleSegmentationLayer (430-440)
  • getMappingInfo (685-692)
frontend/javascripts/viewer/view/context_menu.tsx (7)
frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
  • hideContextMenuAction (243-246)
frontend/javascripts/libs/react_hooks.ts (1)
  • useWkSelector (292-294)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/libs/react_helpers.tsx (1)
  • useFetch (35-52)
frontend/javascripts/admin/rest_api.ts (1)
  • getSegmentSurfaceArea (890-914)
frontend/javascripts/viewer/model/scaleinfo.ts (1)
  • voxelToVolumeInUnit (10-24)
frontend/javascripts/libs/format_utils.ts (2)
  • formatNumberToVolume (299-313)
  • formatNumberToArea (259-273)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (7)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (4)
  • SegmentStatisticsParameters (14-17)
  • SegmentStatisticsParameters (18-20)
  • SegmentStatisticsParametersMeshBased (21-26)
  • SegmentStatisticsParametersMeshBased (27-30)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (3)
  • loadFor (56-65)
  • FullMeshRequest (22-32)
  • FullMeshRequest (34-36)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1)
  • loadFor (40-50)
frontend/javascripts/admin/rest_api.ts (1)
  • getSegmentSurfaceArea (890-914)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
  • getSegmentSurfaceArea (618-649)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/TSAnnotationService.scala (2)
  • findVolume (678-692)
  • baseMappingName (642-651)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)
  • surfaceAreaFromStlBytes (78-114)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (2)
  • WebknossosDataRequest (27-42)
  • WebknossosDataRequest (44-46)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataSource.scala (1)
  • DataSource (31-43)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/rpc/RPCRequest.scala (4)
  • addQueryString (28-31)
  • withTokenFromContext (33-36)
  • silent (64-67)
  • getWithJsonResponse (100-103)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (1)
  • toFox (14-14)
app/controllers/DatasetController.scala (11)
util/src/main/scala/com/scalableminds/util/requestlogging/RequestLogging.scala (1)
  • log (55-62)
app/security/UserAwareRequestLogging.scala (1)
  • log (13-21)
app/security/URLSharing.scala (2)
  • URLSharing (16-24)
  • fallbackTokenAccessContext (18-24)
app/models/dataset/Dataset.scala (2)
  • findOne (203-210)
  • updateForDatasetAndUser (1080-1091)
app/models/user/User.scala (2)
  • findOne (154-161)
  • updateLastActivity (500-507)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (5)
  • Fox (30-230)
  • Fox (232-305)
  • runOptional (159-169)
  • map (281-284)
  • map (377-377)
app/models/dataset/DatasetService.scala (2)
  • dataStoreFor (415-418)
  • publicWrites (571-621)
app/models/analytics/AnalyticsService.scala (1)
  • track (38-46)
app/models/analytics/AnalyticsEvent.scala (2)
  • OpenDatasetEvent (177-191)
  • toJson (31-38)
app/mail/MailchimpClient.scala (1)
  • tagUser (45-54)
app/mail/MailchimpTag.scala (1)
  • MailchimpTag (5-22)
app/controllers/WKRemoteDataStoreController.scala (2)
app/models/dataset/Dataset.scala (1)
  • findOne (203-210)
util/src/main/scala/com/scalableminds/util/accesscontext/AccessContext.scala (1)
  • GlobalAccessContext (25-29)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (6)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (2)
  • SegmentStatisticsParametersMeshBased (21-26)
  • SegmentStatisticsParametersMeshBased (27-30)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (4)
  • DSFullMeshService (38-245)
  • FullMeshRequest (22-32)
  • FullMeshRequest (34-36)
  • loadFor (56-65)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala (2)
  • MeshFileService (61-184)
  • mappingNameForMeshFile (113-125)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DatasetCache.scala (1)
  • getWithLayer (30-36)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (8)
  • Fox (30-230)
  • Fox (232-305)
  • runOptional (159-169)
  • serialCombined (95-99)
  • serialCombined (99-111)
  • map (281-284)
  • map (377-377)
  • toFox (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)
  • surfaceAreaFromStlBytes (78-114)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (6)
frontend/javascripts/libs/react_hooks.ts (1)
  • useWkSelector (292-294)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/admin/rest_api.ts (1)
  • getSegmentSurfaceArea (890-914)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
  • getAdditionalCoordinatesAsString (288-298)
frontend/javascripts/libs/format_utils.ts (1)
  • formatNumberToArea (259-273)
frontend/javascripts/viewer/constants.ts (1)
  • LongUnitToShortUnitMap (469-496)
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/context_menu.tsx

[failure] 1733-1733:
Cannot find name 'requestUrl'.

frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx

[failure] 161-161:
Cannot find name 'requestUrl'.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: backend-tests
  • GitHub Check: build-smoketest-push
🔇 Additional comments (11)
frontend/javascripts/libs/format_utils.ts (1)

296-296: LGTM! Grammar fix in comment.

The article correction from "an volume" to "a volume" improves documentation quality.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DSMeshController.scala (1)

98-98: LGTM! Formatting adjustment.

The trailing blank line addition is a minor formatting change with no functional impact.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (1)

21-30: LGTM! New mesh-based parameters model.

The SegmentStatisticsParametersMeshBased case class appropriately extends the existing parameters to support mesh-based segment statistics workflows. The additional fields (meshFileName and seedPosition) align with the surface area computation feature introduced in this PR, and the JSON formatting follows the established pattern.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1)

75-75: LGTM! Voxel size retrieval now uses webknossos client.

This change aligns with the architectural refactoring that moved voxel size caching from TSRemoteDatastoreClient to TSRemoteWebknossosClient. The functionality remains the same, with the voxel size now retrieved through the centralized webknossos client layer.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1)

18-18: LGTM – voxel size caching centralized in TSRemoteWebknossosClient
No remaining calls to voxelSizeForAnnotationWithCache found; all callers updated accordingly.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (1)

351-378: Surface area endpoint looks good; ensure clients send seedPosition

Endpoint structure and error handling are consistent. It relies on seedPosition for ad-hoc meshing; make sure the frontend includes it (see FE fix).

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (2)

47-48: Good addition: FoxImplicits extension

Extending with FoxImplicits justifies the use of ?~> and .toFox below. No issues.


131-145: Voxel size retrieval via cache looks correct

Fetches datasetId, requests DataSource, lifts voxelSizeOpt with .toFox, and caches per annotation. Token is forwarded. LGTM.

frontend/javascripts/viewer/view/context_menu.tsx (3)

1413-1431: Segment presence checks switched to ‘!== 0’ look good

Using !== 0 consistently for background avoids accidental actions on empty space.

Also applies to: 1432-1434


200-205: Keep menu open on “Load/Reload statistics”

Early-return in onClick handler is a neat way to avoid closing the menu for this item. LGTM.


1831-1833: Minor UI nit: Pushpin spacing update

The margin tweak improves readability; no functional impact. LGTM.

Also applies to: 1844-1846

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/view/context_menu.tsx (1)

1717-1740: Define requestUrl before calling getSegmentSurfaceArea

Fixes “Cannot find name 'requestUrl'”.

       const layerSourceInfo = {
         dataset,
         annotation,
         tracingId,
         segmentationLayerName: visibleSegmentationLayer.name,
       };
+      const requestUrl = getDataOrTracingStoreUrl(layerSourceInfo);
       const magInfo = getMagInfo(visibleSegmentationLayer.mags);
🧹 Nitpick comments (9)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)

78-114: Surface area calculation is mathematically correct.

The STL parsing and area computation logic is sound:

  • Correctly parses binary STL format (80-byte header + 4-byte count + 50-byte triangles)
  • Triangle area via cross product magnitude divided by 2 is accurate
  • tryo wraps the computation to handle exceptions

Consider adding a defensive bounds check at the start to fail fast on malformed input:

 def surfaceAreaFromStlBytes(stlBytes: Array[Byte]): Box[Float] = tryo {
   val dataBuffer = ByteBuffer.wrap(stlBytes)
   dataBuffer.order(ByteOrder.LITTLE_ENDIAN)
   val numberOfTriangles = dataBuffer.getInt(80)
+  val expectedMinBytes = 84 + numberOfTriangles * 50
+  if (stlBytes.length < expectedMinBytes) {
+    throw new IllegalArgumentException(s"STL data too short: expected at least $expectedMinBytes bytes, got ${stlBytes.length}")
+  }
   val normalOffset = 12
   ...

This would provide clearer error messages for malformed mesh data rather than generic buffer underflow exceptions.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (1)

351-377: Clarify why editableMappingTracingId is set to None
Ad-hoc meshing always goes through loadMeshChunkFromAdHoc, which ignores the request’s editableMappingTracingId and uses annotationService.editableMappingLayer when tracing.hasEditableMapping is true. It’s safe to leave it as None, but please add a brief comment explaining this.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)

131-146: Clean cached accessor for voxel size

Nice factoring: cache-backed public method and RPC-based helper with proper token use and Option→Fox lifting. Consider adding a specific error key instead of a string literal for voxelSizeOpt (optional).

frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (2)

1-1: Import only what you need (optional)

To reduce bundle size, prefer per-method lodash import for capitalize or a small custom capitalize.

-import _ from "lodash";
+import capitalize from "lodash/capitalize";

And adjust usage:

-  const capitalizedUnit = _.capitalize(dataSourceUnit);
+  const capitalizedUnit = capitalize(dataSourceUnit);

145-153: Single source of truth for mappingName (optional)

You compute mappingName twice (maybeGetMappingName() and selector mappingName). Use the selector consistently to avoid drift.

-          maybeGetMappingName(),
+          mappingName,

(Apply similarly for bounding boxes.)

Also applies to: 129-130

app/controllers/WKRemoteTracingStoreController.scala (1)

164-172: Add implicit DBAccessContext for consistency.

Other methods in this controller establish an implicit DBAccessContext; consider doing the same here to avoid surprises if DatasetService methods require it later.

 def getDataSource(name: String, key: String, datasetId: ObjectId): Action[AnyContent] =
   Action.async { implicit request =>
-    tracingStoreService.validateAccess(name, key) { _ =>
+    tracingStoreService.validateAccess(name, key) { _ =>
+      implicit val ctx: DBAccessContext = GlobalAccessContext
       for {
-        dataset <- datasetDAO.findOne(datasetId)(GlobalAccessContext) ?~> "dataset.notFound" ~> NOT_FOUND
+        dataset <- datasetDAO.findOne(datasetId)(GlobalAccessContext) ?~> "dataset.notFound" ~> NOT_FOUND
         dataSource <- datasetService.dataSourceFor(dataset)
       } yield Ok(Json.toJson(dataSource))
     }
   }
frontend/javascripts/admin/rest_api.ts (2)

882-888: Include seedPosition in the payload type (parity with backend).

Backend SegmentStatisticsParametersMeshBased supports an optional seedPosition. Add it here to enable ad‑hoc/volume contexts.

 type SegmentStatisticsParametersMeshBased = {
   mag: Vector3;
   segmentIds: number[];
   mappingName?: string | null;
   additionalCoordinates?: AdditionalCoordinate[] | null;
   meshFileName?: string | null;
+  seedPosition?: Vector3 | null;
 };

890-914: Pass through seedPosition and optionally accept it in the function signature.

Surface area for volume tracings may require seedPosition. Thread it through now to avoid another API change.

-export function getSegmentSurfaceArea(
-  requestUrl: string,
-  mag: Vector3,
-  meshFileName: string | undefined | null,
-  segmentIds: Array<number>,
-  additionalCoordinates: AdditionalCoordinate[] | undefined | null,
-  mappingName: string | null | undefined,
-): Promise<number[]> {
+export function getSegmentSurfaceArea(
+  requestUrl: string,
+  mag: Vector3,
+  meshFileName: string | undefined | null,
+  segmentIds: Array<number>,
+  additionalCoordinates: AdditionalCoordinate[] | undefined | null,
+  mappingName: string | null | undefined,
+  seedPosition?: Vector3 | null,
+): Promise<number[]> {
   return doWithToken((token) => {
     const data: SegmentStatisticsParametersMeshBased = {
       mag,
       segmentIds,
       mappingName,
       additionalCoordinates,
       meshFileName,
+      seedPosition,
     };
     return Request.sendJSONReceiveJSON(
       `${requestUrl}/segmentStatistics/surfaceArea?token=${token}`,
       {
         data,
         method: "POST",
       },
     );
   });
 }

Would you like me to propagate this through the current callers (segment stats modal/context menu)?

conf/webknossos.latest.routes (1)

111-114: Expose or remove readDataSource.
The controller defines readDataSource (app/controllers/DatasetController.scala:347) but there’s no corresponding GET route—currently GET /datasets/:datasetId calls read. If you intend to expose readDataSource, add a distinct route; otherwise remove the unused action to avoid confusion.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e95351c and 7d22add.

📒 Files selected for processing (20)
  • app/controllers/DatasetController.scala (1 hunks)
  • app/controllers/WKRemoteDataStoreController.scala (1 hunks)
  • app/controllers/WKRemoteTracingStoreController.scala (1 hunks)
  • conf/messages (1 hunks)
  • conf/webknossos.latest.routes (2 hunks)
  • frontend/javascripts/admin/rest_api.ts (1 hunks)
  • frontend/javascripts/libs/format_utils.ts (1 hunks)
  • frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1 hunks)
  • frontend/javascripts/viewer/view/context_menu.tsx (13 hunks)
  • frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (9 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DSMeshController.scala (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (2 hunks)
  • webknossos-datastore/conf/datastore.latest.routes (1 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (4 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (3 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1 hunks)
  • webknossos-tracingstore/conf/tracingstore.latest.routes (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T13:33:47.097Z
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8824
File: frontend/javascripts/viewer/view/context_menu.tsx:1033-1039
Timestamp: 2025-09-08T13:33:47.097Z
Learning: In frontend/javascripts/viewer/view/context_menu.tsx, the segmentIdLabel intentionally shows the unmapped segment ID (maybeUnmappedSegmentId) in the "within Segment ${id}" text when proofreading is active, as this can be helpful for users. The mapped segment ID (segmentIdAtPosition) is only used as a fallback when the unmapped ID is not available.

Applied to files:

  • frontend/javascripts/viewer/view/context_menu.tsx
🧬 Code graph analysis (12)
app/controllers/WKRemoteDataStoreController.scala (2)
app/models/dataset/Dataset.scala (1)
  • findOne (203-210)
util/src/main/scala/com/scalableminds/util/accesscontext/AccessContext.scala (1)
  • GlobalAccessContext (25-29)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)
  • voxelSizeForAnnotationWithCache (131-135)
app/controllers/DatasetController.scala (7)
app/security/UserAwareRequestLogging.scala (1)
  • log (13-21)
app/security/URLSharing.scala (2)
  • URLSharing (16-24)
  • fallbackTokenAccessContext (18-24)
app/models/dataset/Dataset.scala (2)
  • findOne (203-210)
  • updateForDatasetAndUser (1080-1091)
app/models/user/User.scala (2)
  • findOne (154-161)
  • updateLastActivity (500-507)
app/models/dataset/DatasetService.scala (2)
  • dataStoreFor (415-418)
  • publicWrites (571-621)
app/models/analytics/AnalyticsEvent.scala (2)
  • OpenDatasetEvent (177-191)
  • toJson (31-38)
app/mail/MailchimpClient.scala (1)
  • tagUser (45-54)
frontend/javascripts/admin/rest_api.ts (5)
frontend/javascripts/viewer/constants.ts (1)
  • Vector3 (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (2)
  • AdditionalCoordinate (90-95)
  • AdditionalCoordinate (97-112)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
  • getSegmentSurfaceArea (618-649)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (2)
  • getSegmentSurfaceArea (351-379)
  • data (215-234)
frontend/javascripts/admin/api/token.ts (1)
  • doWithToken (39-74)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (2)
frontend/javascripts/viewer/store.ts (1)
  • WebknossosState (575-595)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (2)
  • getVisibleSegmentationLayer (430-440)
  • getMappingInfo (685-692)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (2)
  • WebknossosDataRequest (27-42)
  • WebknossosDataRequest (44-46)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (7)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (4)
  • SegmentStatisticsParameters (14-17)
  • SegmentStatisticsParameters (18-20)
  • SegmentStatisticsParametersMeshBased (21-26)
  • SegmentStatisticsParametersMeshBased (27-30)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (3)
  • loadFor (56-65)
  • FullMeshRequest (22-32)
  • FullMeshRequest (34-36)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
  • getSegmentSurfaceArea (618-649)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)
  • getAnnotationIdForTracing (91-102)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/TSAnnotationService.scala (2)
  • findVolume (678-692)
  • baseMappingName (642-651)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (7)
  • Fox (30-230)
  • Fox (232-305)
  • serialCombined (95-99)
  • serialCombined (99-111)
  • map (281-284)
  • map (377-377)
  • toFox (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)
  • surfaceAreaFromStlBytes (78-114)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (6)
frontend/javascripts/libs/react_hooks.ts (1)
  • useWkSelector (292-294)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/admin/rest_api.ts (3)
  • getSegmentVolumes (866-880)
  • getSegmentBoundingBoxes (916-930)
  • getSegmentSurfaceArea (890-914)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
  • getAdditionalCoordinatesAsString (288-298)
frontend/javascripts/libs/format_utils.ts (2)
  • formatNumberToVolume (299-313)
  • formatNumberToArea (259-273)
frontend/javascripts/viewer/constants.ts (1)
  • LongUnitToShortUnitMap (469-496)
app/controllers/WKRemoteTracingStoreController.scala (2)
app/controllers/WKRemoteDataStoreController.scala (1)
  • getDataSource (260-270)
app/models/dataset/DatasetService.scala (1)
  • dataSourceFor (390-399)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (6)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (2)
  • SegmentStatisticsParametersMeshBased (21-26)
  • SegmentStatisticsParametersMeshBased (27-30)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (4)
  • DSFullMeshService (38-245)
  • FullMeshRequest (22-32)
  • FullMeshRequest (34-36)
  • loadFor (56-65)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala (3)
  • MeshFileService (61-184)
  • lookUpMeshFileKey (70-75)
  • mappingNameForMeshFile (113-125)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DatasetCache.scala (1)
  • getWithLayer (30-36)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (8)
  • Fox (30-230)
  • Fox (232-305)
  • runOptional (159-169)
  • serialCombined (95-99)
  • serialCombined (99-111)
  • map (281-284)
  • map (377-377)
  • toFox (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)
  • surfaceAreaFromStlBytes (78-114)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataSource.scala (1)
  • DataSource (31-43)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/rpc/RPCRequest.scala (4)
  • addQueryString (28-31)
  • withTokenFromContext (33-36)
  • silent (64-67)
  • getWithJsonResponse (100-103)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (1)
  • toFox (14-14)
frontend/javascripts/viewer/view/context_menu.tsx (7)
frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
  • hideContextMenuAction (243-246)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/libs/react_helpers.tsx (1)
  • useFetch (35-52)
frontend/javascripts/admin/rest_api.ts (1)
  • getSegmentSurfaceArea (890-914)
frontend/javascripts/viewer/model/scaleinfo.ts (1)
  • voxelToVolumeInUnit (10-24)
frontend/javascripts/libs/format_utils.ts (1)
  • formatNumberToArea (259-273)
frontend/javascripts/viewer/constants.ts (1)
  • LongUnitToShortUnitMap (469-496)
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx

[failure] 161-161:
Cannot find name 'requestUrl'.

frontend/javascripts/viewer/view/context_menu.tsx

[failure] 1733-1733:
Cannot find name 'requestUrl'.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: backend-tests
  • GitHub Check: build-smoketest-push
🔇 Additional comments (18)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DSMeshController.scala (1)

98-99: Formatting-only change; LGTM.

Whitespace addition; no functional impact.

conf/messages (1)

279-279: LGTM! Clear error message for mesh loading failures.

The new localization key follows the existing naming pattern and provides a clear error message for mesh-loading failures in the surface area computation workflow.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1)

75-75: LGTM! Clean refactor of voxel size data source.

The voxel size is now consistently retrieved via remoteWebknossosClient instead of remoteDatastoreClient. This consolidation aligns with the broader architecture changes in this PR and maintains the same caching behavior.

webknossos-datastore/conf/datastore.latest.routes (1)

101-101: LGTM! Route follows established patterns.

The new surface area endpoint is consistently placed alongside the existing segment statistics routes (volume and boundingBox) and follows the same URL structure and HTTP method conventions.

frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)

951-960: LGTM! Well-structured mapping name selector.

This new helper provides a clean, centralized way to derive the current mapping name by prioritizing the active volume tracing's mappingName and falling back to the layer-based mappingInfo. This supports the mesh-based surface area workflow introduced in this PR.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (1)

21-30: LGTM! Well-structured parameters for mesh-based statistics.

The new SegmentStatisticsParametersMeshBased case class follows the existing pattern and appropriately extends the base parameters with mesh-specific fields (meshFileName and seedPosition). The JSON formatter is correctly defined.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (1)

290-290: LGTM! Consistent error message key.

The error key has been updated to mesh.loadFull.failed, aligning with the new localization entry in conf/messages for mesh loading failures.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)

618-646: LGTM! Well-implemented mesh-based surface area endpoint.

The new endpoint follows best practices:

  • Proper access control with readDataset permission
  • Defensive check on Line 629 to ensure meshFileName matches the mappingNameForMeshFile before using it
  • Serial processing via Fox.serialCombined prevents concurrent resource contention
  • Clean error handling with ?~> "mesh.loadFull.failed"
  • Appropriate use of editableMappingTracingId = None for dataset-level (non-annotation) context

The implementation is consistent with the parallel tracing store endpoint and follows the established pattern for segment statistics endpoints.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (4)

8-8: Importing FoxImplicits is correct here

Needed for toFox/?~> usage below. LGTM.


12-15: Imports for VoxelSize/DataSource align with new method

Required for the new voxel-size flow. Looks good.


47-48: Mixing in FoxImplicits

Good choice; enables concise error mapping and Option lifting.


58-58: Add cache TTL for voxel size

10-minute TTL is a reasonable default for dataset-derived voxel size.

frontend/javascripts/viewer/view/context_menu.tsx (1)

200-205: Do not close menu on “Load/Reload segment statistics” click

Nice UX guard by checking info?.key === "load-stats". LGTM.

frontend/javascripts/libs/format_utils.ts (1)

296-296: Comment grammar fix looks good.

No functional changes. LGTM.

conf/webknossos.latest.routes (1)

155-156: New tracingstore dataSource route mirrors datastore route.

Looks consistent with WKRemoteDataStoreController.getDataSource. Ensure clients pass the key as query param.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1)

18-18: Import update is fine.

Consistent with getData using WebknossosDataRequest.

webknossos-tracingstore/conf/tracingstore.latest.routes (1)

31-31: Surface area route addition looks correct.

Matches VolumeTracingController.getSegmentSurfaceArea and new frontend call.

app/controllers/WKRemoteDataStoreController.scala (1)

264-264: Good: explicit 404 on missing dataset.

Improves error semantics without changing behavior.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/javascripts/viewer/view/context_menu.tsx (1)

1724-1748: requestUrl is undefined

getSegmentSurfaceArea expects a dataset request URL, but requestUrl is never defined in this scope. This currently breaks the build (Cannot find name 'requestUrl'). Please reuse the URL object you pass to the volume/box calls (e.g., derive it via getRequestUrlForLayerSourceInfo like the other statistics helpers) and pass that variable here.

🧹 Nitpick comments (3)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)

135-145: Consider enhancing the error message with context.

The implementation is correct, but the error message on line 144 could be more informative for debugging by including the annotationId or datasetId.

Apply this diff to enhance the error message:

     for {
       datasetId <- getDatasetIdForAnnotation(annotationId)
       result <- rpc(s"$webknossosUri/api/tracingstores/$tracingStoreName/datasources/$datasetId")
         .addQueryString("key" -> tracingStoreKey)
         .withTokenFromContext
         .silent
         .getWithJsonResponse[DataSource]
-      scale <- result.voxelSizeOpt.toFox ?~> "Could not determine voxel size of dataset"
+      scale <- result.voxelSizeOpt.toFox ?~> s"Could not determine voxel size for annotation $annotationId (dataset $datasetId)"
     } yield scale
app/controllers/WKRemoteDataStoreController.scala (1)

264-266: Use localized message with datasetId for consistency and context.

Prefer passing a resolved Messages(...) with the id to fill the placeholder and align with surrounding code.

-          dataset <- datasetDAO.findOne(datasetId)(GlobalAccessContext) ?~> "dataset.notFound" ~> NOT_FOUND
+          dataset <- datasetDAO.findOne(datasetId)(GlobalAccessContext) ?~> Messages("dataset.notFound", datasetId) ~> NOT_FOUND
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)

78-114: Improve numerical robustness (Double accumulation, skip NaN/Infinity).

  • Accumulate in Double to reduce precision loss on large meshes.
  • Ignore degenerate/invalid triangles that produce NaN/Infinity.
-  def surfaceAreaFromStlBytes(stlBytes: Array[Byte]): Box[Float] = tryo {
+  def surfaceAreaFromStlBytes(stlBytes: Array[Byte]): Box[Float] = tryo {
     val dataBuffer = ByteBuffer.wrap(stlBytes)
     dataBuffer.order(ByteOrder.LITTLE_ENDIAN)
     val numberOfTriangles = dataBuffer.getInt(80)
     val normalOffset = 12
-    var surfaceSumMutable = 0.0f
+    var surfaceSumMutable = 0.0d
     val headerOffset = 84
     val bytesPerTriangle = 50
     for (triangleIndex <- 0 until numberOfTriangles) {
       val triangleVerticesOffset = headerOffset + triangleIndex * bytesPerTriangle + normalOffset
       val v1x = dataBuffer.getFloat(triangleVerticesOffset + 4 * 0)
       val v1y = dataBuffer.getFloat(triangleVerticesOffset + 4 * 1)
       val v1z = dataBuffer.getFloat(triangleVerticesOffset + 4 * 2)
       val v2x = dataBuffer.getFloat(triangleVerticesOffset + 4 * 3)
       val v2y = dataBuffer.getFloat(triangleVerticesOffset + 4 * 4)
       val v2z = dataBuffer.getFloat(triangleVerticesOffset + 4 * 5)
       val v3x = dataBuffer.getFloat(triangleVerticesOffset + 4 * 6)
       val v3y = dataBuffer.getFloat(triangleVerticesOffset + 4 * 7)
       val v3z = dataBuffer.getFloat(triangleVerticesOffset + 4 * 8)

       val vec1x = v2x - v1x
       val vec1y = v2y - v1y
       val vec1z = v2z - v1z
       val vec2x = v3x - v1x
       val vec2y = v3y - v1y
       val vec2z = v3z - v1z

       val crossx = vec1y * vec2z - vec1z * vec2y
       val crossy = vec1z * vec2x - vec1x * vec2z
       val crossz = vec1x * vec2y - vec1y * vec2x

-      val magnitude = Math.sqrt(crossx * crossx + crossy * crossy + crossz * crossz).toFloat
-
-      surfaceSumMutable = surfaceSumMutable + (magnitude / 2.0f)
+      val magnitude = math.sqrt(
+        crossx.toDouble * crossx.toDouble +
+        crossy.toDouble * crossy.toDouble +
+        crossz.toDouble * crossz.toDouble
+      )
+      if (!magnitude.isNaN && !magnitude.isInfinite) {
+        surfaceSumMutable += magnitude / 2.0
+      }
     }
-    surfaceSumMutable
+    surfaceSumMutable.toFloat
   }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e95351c and c61dbca.

📒 Files selected for processing (21)
  • app/controllers/DatasetController.scala (1 hunks)
  • app/controllers/WKRemoteDataStoreController.scala (1 hunks)
  • app/controllers/WKRemoteTracingStoreController.scala (1 hunks)
  • conf/messages (1 hunks)
  • conf/webknossos.latest.routes (2 hunks)
  • frontend/javascripts/admin/rest_api.ts (1 hunks)
  • frontend/javascripts/libs/format_utils.ts (1 hunks)
  • frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1 hunks)
  • frontend/javascripts/viewer/view/context_menu.tsx (13 hunks)
  • frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (9 hunks)
  • unreleased_changes/8928.md (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DSMeshController.scala (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (2 hunks)
  • webknossos-datastore/conf/datastore.latest.routes (1 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (4 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (3 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1 hunks)
  • webknossos-tracingstore/conf/tracingstore.latest.routes (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)
  • voxelSizeForAnnotationWithCache (131-135)
app/controllers/DatasetController.scala (7)
app/security/UserAwareRequestLogging.scala (1)
  • log (13-21)
app/security/URLSharing.scala (2)
  • URLSharing (16-24)
  • fallbackTokenAccessContext (18-24)
app/models/dataset/Dataset.scala (2)
  • findOne (203-210)
  • updateForDatasetAndUser (1080-1091)
app/models/user/User.scala (2)
  • findOne (154-161)
  • updateLastActivity (500-507)
app/models/dataset/DatasetService.scala (2)
  • dataStoreFor (415-418)
  • publicWrites (571-621)
app/models/analytics/AnalyticsEvent.scala (2)
  • OpenDatasetEvent (177-191)
  • toJson (31-38)
app/mail/MailchimpClient.scala (1)
  • tagUser (45-54)
app/controllers/WKRemoteTracingStoreController.scala (3)
app/controllers/WKRemoteDataStoreController.scala (1)
  • getDataSource (260-270)
app/models/annotation/TracingStore.scala (1)
  • validateAccess (47-58)
app/models/dataset/DatasetService.scala (1)
  • dataSourceFor (390-399)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (2)
frontend/javascripts/viewer/store.ts (1)
  • WebknossosState (575-595)
frontend/javascripts/viewer/model/accessors/dataset_accessor.ts (2)
  • getVisibleSegmentationLayer (430-440)
  • getMappingInfo (685-692)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (6)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (4)
  • SegmentStatisticsParameters (14-17)
  • SegmentStatisticsParameters (18-20)
  • SegmentStatisticsParametersMeshBased (21-26)
  • SegmentStatisticsParametersMeshBased (27-30)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (3)
  • loadFor (56-65)
  • FullMeshRequest (22-32)
  • FullMeshRequest (34-36)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
  • getSegmentSurfaceArea (618-649)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/TSAnnotationService.scala (2)
  • findVolume (678-692)
  • baseMappingName (642-651)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (7)
  • Fox (30-230)
  • Fox (232-305)
  • serialCombined (95-99)
  • serialCombined (99-111)
  • map (281-284)
  • map (377-377)
  • toFox (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)
  • surfaceAreaFromStlBytes (78-114)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (6)
frontend/javascripts/libs/react_hooks.ts (1)
  • useWkSelector (292-294)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/admin/rest_api.ts (3)
  • getSegmentVolumes (866-880)
  • getSegmentBoundingBoxes (916-930)
  • getSegmentSurfaceArea (890-914)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
  • getAdditionalCoordinatesAsString (288-298)
frontend/javascripts/libs/format_utils.ts (2)
  • formatNumberToVolume (299-313)
  • formatNumberToArea (259-273)
frontend/javascripts/viewer/constants.ts (1)
  • LongUnitToShortUnitMap (469-496)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (8)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (2)
  • SegmentStatisticsParametersMeshBased (21-26)
  • SegmentStatisticsParametersMeshBased (27-30)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/AccessTokenService.scala (4)
  • webknossos (70-72)
  • validateAccessFromTokenContext (87-94)
  • UserAccessRequest (30-30)
  • UserAccessRequest (31-72)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (4)
  • DSFullMeshService (38-245)
  • FullMeshRequest (22-32)
  • FullMeshRequest (34-36)
  • loadFor (56-65)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala (3)
  • MeshFileService (61-184)
  • lookUpMeshFileKey (70-75)
  • mappingNameForMeshFile (113-125)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DatasetCache.scala (1)
  • getWithLayer (30-36)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/Hdf5MeshFileService.scala (1)
  • mappingNameForMeshFile (21-30)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/ZarrMeshFileService.scala (1)
  • mappingNameForMeshFile (97-103)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)
  • surfaceAreaFromStlBytes (78-114)
frontend/javascripts/admin/rest_api.ts (5)
frontend/javascripts/viewer/constants.ts (1)
  • Vector3 (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (2)
  • AdditionalCoordinate (90-95)
  • AdditionalCoordinate (97-112)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
  • getSegmentSurfaceArea (618-649)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (2)
  • getSegmentSurfaceArea (351-379)
  • data (215-234)
frontend/javascripts/admin/api/token.ts (1)
  • doWithToken (39-74)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (4)
frontend/javascripts/types/api_types.ts (1)
  • VoxelSize (144-147)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataSource.scala (1)
  • DataSource (31-43)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/rpc/RPCRequest.scala (4)
  • addQueryString (28-31)
  • withTokenFromContext (33-36)
  • silent (64-67)
  • getWithJsonResponse (100-103)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (1)
  • toFox (14-14)
frontend/javascripts/viewer/view/context_menu.tsx (5)
frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
  • hideContextMenuAction (243-246)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/libs/react_helpers.tsx (1)
  • useFetch (35-52)
frontend/javascripts/admin/rest_api.ts (1)
  • getSegmentSurfaceArea (890-914)
frontend/javascripts/libs/format_utils.ts (2)
  • formatNumberToVolume (299-313)
  • formatNumberToArea (259-273)
app/controllers/WKRemoteDataStoreController.scala (2)
app/models/dataset/Dataset.scala (1)
  • findOne (203-210)
util/src/main/scala/com/scalableminds/util/accesscontext/AccessContext.scala (1)
  • GlobalAccessContext (25-29)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (2)
  • WebknossosDataRequest (27-42)
  • WebknossosDataRequest (44-46)
🪛 GitHub Check: frontend-tests
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx

[failure] 161-161:
Cannot find name 'requestUrl'.

frontend/javascripts/viewer/view/context_menu.tsx

[failure] 1733-1733:
Cannot find name 'requestUrl'.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-smoketest-push
  • GitHub Check: backend-tests
🔇 Additional comments (14)
unreleased_changes/8928.md (1)

1-2: Changelog entry looks good

Entry follows the usual format and clearly announces the new surface area statistic.

frontend/javascripts/libs/format_utils.ts (1)

296-296: Comment typo fix looks good.

Thanks for correcting the grammar in this doc comment. 👍

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1)

75-75: LGTM! Caching improves performance.

Switching to voxelSizeForAnnotationWithCache adds caching for voxel size lookups, which will improve performance for repeated ad-hoc mesh operations on the same annotation. The error handling and return type remain consistent.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (6)

47-47: LGTM!

The FoxImplicits mixin is correctly added to support the .toFox conversion used in line 144.


58-58: LGTM!

The voxel size cache with a 10-minute TTL is appropriate, as voxel sizes are immutable dataset properties.


131-133: LGTM!

The cache wrapper method correctly delegates to the private implementation and follows the established caching pattern used elsewhere in the class.


8-8: LGTM!

The new imports and FoxImplicits mixin appropriately support the voxel size caching functionality.

Also applies to: 12-12, 14-14, 47-47


58-58: LGTM!

The voxel size cache with a 10-minute TTL is appropriate for dataset properties that change infrequently.


131-145: LGTM!

The voxel size retrieval logic is well-structured:

  • Proper caching with annotationId as the key (appropriate since voxel size is a dataset property).
  • Implicit parameters are correctly captured in the cache callback lambda.
  • Error handling ensures a clear message when voxel size cannot be determined.
  • Good encapsulation with the private helper method.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (2)

18-18: Removed voxel size methods have no dangling invocations. TSRemoteDatastoreClient is clean; existing calls to voxelSizeForAnnotationWithCache now correctly target TSRemoteWebknossosClient.


18-18: LGTM!

The import simplification appropriately reflects the removal of voxel size caching logic from this file.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DSMeshController.scala (1)

98-98: No functional change — whitespace only.

frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)

951-960: Helper looks good.

Derives the mapping name with sensible fallbacks; integrates with existing accessors.

conf/messages (1)

279-280: New i18n key OK.

Key meshes well with existing mesh.* messages and improves error clarity.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (3)
frontend/javascripts/admin/rest_api.ts (1)

882-888: Missing seedPosition in payload breaks tracingstore ad‑hoc meshing

The tracingstore endpoint requires a seed to build ad‑hoc meshes. The TS type and payload omit it, so volume annotations will fail.

Apply this diff:

 type SegmentStatisticsParametersMeshBased = {
   mag: Vector3;
   segmentIds: number[];
   mappingName?: string | null;
   additionalCoordinates?: AdditionalCoordinate[] | null;
   meshFileName?: string | null;
+  seedPosition?: Vector3 | null;
 };

 export function getSegmentSurfaceArea(
   layerSourceInfo: LayerSourceInfo,
   mag: Vector3,
   meshFileName: string | undefined | null,
   segmentIds: Array<number>,
   additionalCoordinates: AdditionalCoordinate[] | undefined | null,
   mappingName: string | null | undefined,
+  seedPosition?: Vector3 | null | undefined,
 ): Promise<number[]> {
   const requestUrl = getDataOrTracingStoreUrl(layerSourceInfo);
   return doWithToken((token) => {
     const data: SegmentStatisticsParametersMeshBased = {
       mag,
       segmentIds,
       mappingName,
       additionalCoordinates,
       meshFileName,
+      seedPosition,
     };
     return Request.sendJSONReceiveJSON(
       `${requestUrl}/segmentStatistics/surfaceArea?token=${token}`,
       {
         data,
         method: "POST",
       },
     );
   });
 }
#!/bin/bash
# Find and show all call sites to update with the new seedPosition argument.
rg -n --type=ts -C2 '\bgetSegmentSurfaceArea\s*\('

Also applies to: 890-915

frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (1)

31-34: CSV header typo: “segmendId” → “segmentId”

Fix the column name.

-  return `segmendId,segmentName,groupId,groupName,volumeInVoxel,volumeIn${capitalizedUnit}3,surfaceAreaIn${capitalizedUnit}2,boundingBoxTopLeftPositionX,boundingBoxTopLeftPositionY,boundingBoxTopLeftPositionZ,boundingBoxSizeX,boundingBoxSizeY,boundingBoxSizeZ`;
+  return `segmentId,segmentName,groupId,groupName,volumeInVoxel,volumeIn${capitalizedUnit}3,surfaceAreaIn${capitalizedUnit}2,boundingBoxTopLeftPositionX,boundingBoxTopLeftPositionY,boundingBoxTopLeftPositionZ,boundingBoxSizeX,boundingBoxSizeY,boundingBoxSizeZ`;
frontend/javascripts/viewer/view/context_menu.tsx (1)

1899-1938: Duplicate keys in info items

Both rows use key "volumeInfo". Make keys unique.

-      getInfoMenuItem(
-        "volumeInfo",
+      getInfoMenuItem(
+        "surfaceAreaInfo",
         <>
           <i className="segment-context-icon">m²</i>
           Surface Area: {segmentSurfaceAreaLabel}
           {copyIconWithTooltip(segmentSurfaceAreaLabel as string, "Copy surface area")}
         </>,
       ),
🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (1)

136-143: Use the same mappingName source for all three requests

Surface area uses mappingName while volume/bbox use maybeGetMappingName(). Align them to avoid mismatches.

-          mappingName,
+          maybeGetMappingName(),

Also applies to: 160-167

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c61dbca and e9ec82e.

📒 Files selected for processing (3)
  • frontend/javascripts/admin/rest_api.ts (1 hunks)
  • frontend/javascripts/viewer/view/context_menu.tsx (13 hunks)
  • frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (9 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/javascripts/admin/rest_api.ts (6)
frontend/javascripts/viewer/constants.ts (1)
  • Vector3 (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (2)
  • AdditionalCoordinate (90-95)
  • AdditionalCoordinate (97-112)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
  • getSegmentSurfaceArea (618-649)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (2)
  • getSegmentSurfaceArea (351-379)
  • data (215-234)
frontend/javascripts/viewer/model/bucket_data_handling/wkstore_helper.ts (2)
  • LayerSourceInfo (4-10)
  • getDataOrTracingStoreUrl (12-20)
frontend/javascripts/admin/api/token.ts (1)
  • doWithToken (39-74)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (4)
frontend/javascripts/libs/react_hooks.ts (1)
  • useWkSelector (292-294)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/admin/rest_api.ts (3)
  • getSegmentVolumes (866-880)
  • getSegmentBoundingBoxes (917-931)
  • getSegmentSurfaceArea (890-915)
frontend/javascripts/libs/format_utils.ts (1)
  • formatNumberToArea (259-273)
frontend/javascripts/viewer/view/context_menu.tsx (5)
frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
  • hideContextMenuAction (243-246)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/libs/react_helpers.tsx (1)
  • useFetch (35-52)
frontend/javascripts/admin/rest_api.ts (1)
  • getSegmentSurfaceArea (890-915)
frontend/javascripts/libs/format_utils.ts (1)
  • formatNumberToArea (259-273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: backend-tests
  • GitHub Check: build-smoketest-push
  • GitHub Check: frontend-tests

Comment on lines 160 to 167
getSegmentSurfaceArea(
storeInfoType,
layersFinestMag,
currentMeshFile?.name,
segmentIds,
additionalCoordinates,
mappingName,
),
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Tracingstore path needs a seed; modal currently provides none

For volume annotations (tracingstore URL), the surface‑area endpoint expects seedPosition. The modal has no obvious seed and this call will likely 4xx/5xx.

Options:

  • Gate area fetching here to dataset/static layers (meshFile available) and rely on the context menu (which has a position) to fetch area for tracings.
  • Or restructure to compute seeds (e.g., derive per‑segment from bounding boxes) and pass them; the API currently accepts only one seed for all IDs.

Do you want me to submit a follow‑up patch that gates this call on datastore/static layers and leaves tracingstore area to the context menu flow?


🏁 Script executed:

#!/bin/bash
set -e
echo "Searching for getSegmentSurfaceArea definition and references..."
rg -n "getSegmentSurfaceArea" -C3
echo
echo "Searching for seedPosition usage..."
rg -n "seedPosition" -C3

Length of output: 58605


🏁 Script executed:

#!/bin/bash
echo "Showing getSegmentSurfaceArea implementation and related types..."
sed -n '880,920p' frontend/javascripts/admin/rest_api.ts
echo
echo "Searching for SegmentStatisticsParametersMeshBased interface..."
rg -n "SegmentStatisticsParametersMeshBased" -C3 frontend/javascripts/admin/rest_api.ts

Length of output: 1609


Gate surface-area fetch to static layers
The modal omits seedPosition, causing the tracingstore /segmentStatistics/surfaceArea call to 4xx. Wrap getSegmentSurfaceArea in a currentMeshFile (static layer) check or supply a valid seed.

🤖 Prompt for AI Agents
In
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx
around lines 160–167, the getSegmentSurfaceArea invocation is missing
seedPosition and is being called for dynamic layers, causing the tracingstore
/segmentStatistics/surfaceArea request to 4xx; either wrap the call so it only
runs when currentMeshFile is truthy (i.e., it's a static layer) or pass a valid
seedPosition into getSegmentSurfaceArea (pull seedPosition from the modal/store
props or compute it before calling). Ensure the surface-area fetch is only
invoked for static layers or include the seedPosition argument so the
tracingstore request succeeds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's still a problem I think @fm3.

One of the core problems is that the request does not allow to pass a seed position for every segment in case multiple segments are requested in one go. Or is this field unused? Bc. at least in the datastore DataSourceController -> getSegmentSurfaceArea -> fullMeshService.loadFor -> loadFullMeshFromAdHoc -> fullMeshRequest.seedPosition.toFox has an assertion that a seed position is passed. Moreover, I think it should be a valid position and not just any. Thus, a list of seedPositions would be needed, or am I incorrect here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed in https://scm.slack.com/archives/CMBMU5684/p1760434512682859?thread_ts=1759836269.384579&cid=CMBMU5684 the seedPosition is not required. positions will instead be taken from segment index file. if there isn’t one, no segment stats are offered at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://scm.slack.com/archives/CMBMU5684/p1760434271069449?thread_ts=1759836269.384579&cid=CMBMU5684 for further replies.

tl;dr: Surface area calculation is only supported in case a segmentation index exists. Otherwise, it is not. And the erroneous code path is in case no segment index exists. Thus, it should be safe, and no seed position should be needed.

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hej here is my code feedback :D

datasetLastUsedTimesDAO.updateForDatasetAndUser(dataset._id, user._id))
// Access checked above via dataset. In case of shared dataset/annotation, show datastore even if not otherwise accessible
dataStore <- datasetService.dataStoreFor(dataset)(GlobalAccessContext)
js <- datasetService.publicWrites(dataset, request.identity, Some(organization), Some(dataStore))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a side note: The current publicWrites code leaks team structure to outside organization users in case of a public / shared dataset. This might be unwanted

);
}

type SegmentStatisticsParametersMeshBased = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rename this to SurfaceAreaParametersMeshBased or so as at least the route sounds like that's whats returned by the route, and not all segment stats

// Segment 0 cannot/shouldn't be made active (as this
// would be an eraser effectively).
segmentIdAtPosition > 0 && !disabledVolumeInfo.PICK_CELL.isDisabled
segmentIdAtPosition !== 0 && !disabledVolumeInfo.PICK_CELL.isDisabled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure why this change was made. Logic should stay the same unless the segment id stays in the expected [0,...] reange. But the previous code guarded even against the unexpected case of a negative segment id. So it was safer than this version imo. So unsure why this change was made.

Same below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why this change was made @fm3?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I don’t know. Maybe negative sgement ids are now used in the frontend to express some special case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo no need to resovle this before merging as this is no critical change. Can be answered later

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WK supports negative segment ids by now, but the code here did not respect this. therefore, I fixed this.

object SegmentStatisticsParameters {
implicit val jsonFormat: OFormat[SegmentStatisticsParameters] = Json.format[SegmentStatisticsParameters]
}
case class SegmentStatisticsParametersMeshBased(mag: Vec3Int,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rename this here as well as this is only used for the surface route 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning for this naming was that the other two segment statistics routes share a paramter type, which means it is named SegmentStatisticsParameters. And this one will probably soon share its paramter type with the sphericity route, which will also be mesh-based. Hence the distinction between SegmentStatisticsParameters (for the segment statistic routes that are not mesh-based) and SegmentStatisticsParametersMeshBased (for the ones that are). But you are right, currently the latter category only has one member, so we could also rename it if you think it’s worth it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I was unaware of another planned route which will use the same parameters. So I guess this is fine as long as the sphericity route will be there in the relatively near future. If its planned for .e.g in one year, the renaming would still make sense imo.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (2)

33-33: Fix CSV header typo: "segmendId" → "segmentId".

This typo was flagged in a previous review but remains unfixed.

-  return `segmendId,segmentName,groupId,groupName,volumeInVoxel,volumeIn${capitalizedUnit}3,surfaceAreaIn${capitalizedUnit}2,boundingBoxTopLeftPositionX,boundingBoxTopLeftPositionY,boundingBoxTopLeftPositionZ,boundingBoxSizeX,boundingBoxSizeY,boundingBoxSizeZ`;
+  return `segmentId,segmentName,groupId,groupName,volumeInVoxel,volumeIn${capitalizedUnit}3,surfaceAreaIn${capitalizedUnit}2,boundingBoxTopLeftPositionX,boundingBoxTopLeftPositionY,boundingBoxTopLeftPositionZ,boundingBoxSizeX,boundingBoxSizeY,boundingBoxSizeZ`;

160-167: Surface area call may fail for volume annotations without mesh files.

When currentMeshFile is null (e.g., for volume annotations), the backend may reject the request or return invalid results. A previous review flagged that tracingstore endpoints require seedPosition for ad-hoc mesh generation, which this modal doesn't provide.

Wrap the getSegmentSurfaceArea call in a conditional check:

       const segmentStatisticsObjects = await Promise.all([
         getSegmentVolumes(
           storeInfoType,
           layersFinestMag,
           segmentIds,
           additionalCoordinates,
           maybeGetMappingName(),
         ),
         getSegmentBoundingBoxes(
           storeInfoType,
           layersFinestMag,
           segmentIds,
           additionalCoordinates,
           maybeGetMappingName(),
         ),
-        getSegmentSurfaceArea(
-          storeInfoType,
-          layersFinestMag,
-          currentMeshFile?.name,
-          segmentIds,
-          additionalCoordinates,
-          mappingName,
-        ),
+        currentMeshFile
+          ? getSegmentSurfaceArea(
+              storeInfoType,
+              layersFinestMag,
+              currentMeshFile.name,
+              segmentIds,
+              additionalCoordinates,
+              mappingName,
+            )
+          : Promise.resolve(segmentIds.map(() => 0)),
       ]).then(

This ensures surface area is only fetched when a mesh file is available, falling back to zeros for volume annotations. Adjust the UI to indicate when surface area is unavailable if needed.

🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (1)

7-7: Consider native string capitalization instead of lodash.

Lodash is imported solely for _.capitalize() at line 32. A native alternative removes the dependency:

-import _ from "lodash";

Then at line 32:

-  const capitalizedUnit = _.capitalize(dataSourceUnit);
+  const capitalizedUnit = dataSourceUnit.charAt(0).toUpperCase() + dataSourceUnit.slice(1);

This eliminates the lodash dependency for a trivial operation and reduces bundle size.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d1cb2bc and 299c8e3.

📒 Files selected for processing (1)
  • frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (6)
frontend/javascripts/libs/react_hooks.ts (1)
  • useWkSelector (292-294)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/admin/rest_api.ts (3)
  • getSegmentVolumes (866-880)
  • getSegmentBoundingBoxes (917-931)
  • getSegmentSurfaceArea (890-915)
frontend/javascripts/viewer/model/accessors/flycam_accessor.ts (1)
  • getAdditionalCoordinatesAsString (288-298)
frontend/javascripts/libs/format_utils.ts (2)
  • formatNumberToVolume (299-313)
  • formatNumberToArea (259-273)
frontend/javascripts/viewer/constants.ts (1)
  • LongUnitToShortUnitMap (469-496)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: backend-tests
  • GitHub Check: build-smoketest-push
  • GitHub Check: frontend-tests
🔇 Additional comments (2)
frontend/javascripts/viewer/view/right-border-tabs/segments_tab/segment_statistics_modal.tsx (2)

57-58: LGTM!

The new surfaceAreaInUnit2 and formattedSurfaceArea fields follow the established pattern for volumeInUnit3 and formattedSize. The types are correct.


172-207: LGTM!

Surface area response processing mirrors the established patterns for volume and bounding box. The use of formatNumberToArea is consistent with formatNumberToVolume, and the data flow is correct.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)

627-643: Fix type comparison and hardcoded mappingType (duplicate from past reviews).

Two critical issues remain unaddressed from previous reviews:

Issue 1 (Line 629): The condition mappingNameForMeshFile.contains(request.body.meshFileName) will always be false. Option.contains() expects a String, but request.body.meshFileName is Option[String]. This causes static mesh files to never be used, forcing ad-hoc meshing even when a valid precomputed mesh file exists.

Issue 2 (Line 633): The mappingType is hardcoded to "HDF5" whenever mappingName exists. For zarr-based mesh files and other formats, this forces DSFullMeshService to use the wrong reader, causing failures.

Apply this diff to fix both issues:

           surfaceAreas <- Fox.serialCombined(request.body.segmentIds) { segmentId =>
+            val useProvidedMesh = meshFileKeyOpt.isDefined &&
+              mappingNameForMeshFile == request.body.mappingName
             val fullMeshRequest = FullMeshRequest(
-              meshFileName =
-                if (mappingNameForMeshFile.contains(request.body.meshFileName)) request.body.meshFileName else None,
+              meshFileName = if (useProvidedMesh) request.body.meshFileName else None,
               lod = None,
               segmentId = segmentId,
               mappingName = request.body.mappingName,
-              mappingType = request.body.mappingName.map(_ => "HDF5"),
+              mappingType = None, // DSFullMeshService will determine the correct type
               editableMappingTracingId = None,
               mag = Some(request.body.mag),
               seedPosition = request.body.seedPosition,
               additionalCoordinates = request.body.additionalCoordinates,
             )

Alternatively, if you need to pass the actual mapping type, derive it from the mesh file key's data format rather than hardcoding it.

🧹 Nitpick comments (1)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)

131-145: Parse JSON as UsableDataSource
Replace .getWithJsonResponse[DataSource] with .getWithJsonResponse[UsableDataSource] in voxelSizeForAnnotation to enforce that only usable datasources are deserialized (as in getDataSourceForAnnotation). Confirm the /datasources/$datasetId endpoint never returns an unusable datasource.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10cfcdd and f39564b.

📒 Files selected for processing (3)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (7)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (2)
  • SegmentStatisticsParametersMeshBased (21-26)
  • SegmentStatisticsParametersMeshBased (27-30)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/AccessTokenService.scala (4)
  • webknossos (70-72)
  • validateAccessFromTokenContext (87-94)
  • UserAccessRequest (30-30)
  • UserAccessRequest (31-72)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (4)
  • DSFullMeshService (38-245)
  • FullMeshRequest (22-32)
  • FullMeshRequest (34-36)
  • loadFor (56-65)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala (2)
  • MeshFileService (61-184)
  • mappingNameForMeshFile (113-125)
frontend/javascripts/admin/rest_api.ts (1)
  • getSegmentSurfaceArea (890-915)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DatasetCache.scala (1)
  • getWithLayer (30-36)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)
  • surfaceAreaFromStlBytes (78-114)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataSource.scala (1)
  • DataSource (31-43)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/rpc/RPCRequest.scala (3)
  • withTokenFromContext (73-76)
  • silent (104-107)
  • getWithJsonResponse (132-135)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (1)
  • toFox (14-14)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/DataRequests.scala (2)
  • WebknossosDataRequest (27-42)
  • WebknossosDataRequest (44-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: frontend-tests
  • GitHub Check: build-smoketest-push
  • GitHub Check: backend-tests
🔇 Additional comments (8)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3)

21-21: LGTM!

The new imports for mesh-based segment statistics are correctly added and used in the getSegmentSurfaceArea endpoint.

Also applies to: 27-32


77-77: LGTM!

The fullMeshService dependency is properly injected and used in the getSegmentSurfaceArea endpoint.


618-622: LGTM!

The method signature, access validation, and data source retrieval follow the established patterns from other segment statistics endpoints.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (2)

18-18: LGTM! Import consolidation looks correct.

The import change to WebknossosDataRequest is appropriate and aligns with its usage in the getData method (line 49).


29-148: Ignore removal check for voxelSizeForAnnotationWithCache: this PR doesn’t remove or modify that API in TSRemoteWebknossosClient.scala, so there’s no breakage in TSRemoteDatastoreClient.scala.

Likely an incorrect or invalid review comment.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (3)

8-8: LGTM: Necessary imports for new voxel size functionality.

The new imports support the voxel size caching feature:

  • FoxImplicits enables .toFox conversion (line 144)
  • VoxelSize is the return type for the new methods
  • DataSource is used for deserializing the RPC response

Also applies to: 12-12, 14-14


47-47: LGTM: FoxImplicits mixin required for Option → Fox conversion.

This mixin provides the implicit conversion used on line 144 where Option[VoxelSize] is converted to Fox[VoxelSize].


58-58: LGTM: Cache TTL is appropriate for voxel size data.

The 10-minute TTL is reasonable since voxel sizes rarely change after dataset creation. The cache follows the same pattern as other caches in the class.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)

58-58: Consider caching by datasetId instead of annotationId.

The cache uses annotationId as the key, but voxel size is a dataset property, not an annotation property. Multiple annotations referencing the same dataset will create duplicate cache entries, wasting memory. Since line 138 already retrieves the datasetId, consider refactoring to cache by datasetId for better efficiency.

Example refactor:

-private lazy val voxelSizeCache: AlfuCache[ObjectId, VoxelSize] = AlfuCache(timeToLive = 10 minutes)
+private lazy val voxelSizeByDatasetCache: AlfuCache[ObjectId, VoxelSize] = AlfuCache(timeToLive = 10 minutes)

Then in the public method:

def voxelSizeForAnnotationWithCache(annotationId: ObjectId)(implicit tc: TokenContext,
                                                            ec: ExecutionContext): Fox[VoxelSize] =
  for {
    datasetId <- getDatasetIdForAnnotation(annotationId)
    voxelSize <- voxelSizeByDatasetCache.getOrLoad(datasetId, dId => voxelSizeForDataset(dId))
  } yield voxelSize

private def voxelSizeForDataset(datasetId: ObjectId)(implicit tc: TokenContext,
                                                     ec: ExecutionContext): Fox[VoxelSize] =
  for {
    result <- rpc(s"$webknossosUri/api/tracingstores/$tracingStoreName/datasources/$datasetId")
      .addQueryParam("key", tracingStoreKey)
      .withTokenFromContext
      .silent
      .getWithJsonResponse[DataSource]
    scale <- result.voxelSizeOpt.toFox ?~> "Could not determine voxel size of dataset"
  } yield scale
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f39564b and 7b44303.

📒 Files selected for processing (1)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (3)
frontend/javascripts/types/api_types.ts (1)
  • VoxelSize (144-147)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataSource.scala (1)
  • DataSource (31-43)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/rpc/RPCRequest.scala (15)
  • addQueryParam (30-33)
  • addQueryParam (33-36)
  • addQueryParam (36-39)
  • addQueryParam (39-42)
  • addQueryParam (42-45)
  • addQueryParam (45-50)
  • addQueryParam (50-53)
  • addQueryParam (53-56)
  • addQueryParam (56-59)
  • addQueryParam (59-63)
  • addQueryParam (63-68)
  • addQueryParam (68-71)
  • withTokenFromContext (73-76)
  • silent (104-107)
  • getWithJsonResponse (132-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-smoketest-push
  • GitHub Check: frontend-tests
  • GitHub Check: backend-tests
🔇 Additional comments (2)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (2)

8-8: LGTM! Imports and FoxImplicits extension are appropriate.

The new imports and FoxImplicits extension are necessary for the voxel size caching functionality and follow the existing patterns in the codebase.

Also applies to: 12-12, 14-14, 47-47


131-145: No changes required: both UsableDataSource and UnusableDataSource override voxelSizeOpt, so the existing .toFox handles missing voxel sizes correctly.

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still have a problem here. See my comment below.

Moreover, I think a comment of mine was lost. At least I cannot find it anymore.

My comment was regarding TSRemoteWebknossosClient -> voxelSizeForAnnotation. Which uses the tracingstores/$tracingStoreName/datasources/$datasetId route. This is the only place where it is used (as far as I can remember) and it directly throws everything away from the json except the voxelSize. So imo the route should thus only return the voxel size and renamed to something like tracingstores/$tracingStoreName/datasources/$datasetId/voxelSize or so. But as this was not really changed in this pr, this should be something for a follow up. Or what do you think?

Comment on lines 160 to 167
getSegmentSurfaceArea(
storeInfoType,
layersFinestMag,
currentMeshFile?.name,
segmentIds,
additionalCoordinates,
mappingName,
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's still a problem I think @fm3.

One of the core problems is that the request does not allow to pass a seed position for every segment in case multiple segments are requested in one go. Or is this field unused? Bc. at least in the datastore DataSourceController -> getSegmentSurfaceArea -> fullMeshService.loadFor -> loadFullMeshFromAdHoc -> fullMeshRequest.seedPosition.toFox has an assertion that a seed position is passed. Moreover, I think it should be a valid position and not just any. Thus, a list of seedPositions would be needed, or am I incorrect here?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
frontend/javascripts/viewer/view/context_menu.tsx (1)

1692-1758: LGTM! Three-value loading flow is well-implemented.

The extended fetching logic correctly handles the three statistics (volume, bounding box, surface area) with consistent tuple handling throughout. The dependencies are correct, triggering re-fetch when needed.

One optional improvement: the error handling returns the same error message for all three statistics (lines 1750-1751). Consider more granular error handling to indicate which specific statistic(s) failed to fetch, though the current approach is acceptable for user experience.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c684cd and 89ed12c.

📒 Files selected for processing (2)
  • frontend/javascripts/admin/rest_api.ts (1 hunks)
  • frontend/javascripts/viewer/view/context_menu.tsx (13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/javascripts/admin/rest_api.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/javascripts/viewer/view/context_menu.tsx (6)
frontend/javascripts/viewer/model/actions/ui_actions.ts (1)
  • hideContextMenuAction (243-246)
frontend/javascripts/viewer/model/accessors/volumetracing_accessor.ts (1)
  • getCurrentMappingName (951-960)
frontend/javascripts/libs/react_helpers.tsx (1)
  • useFetch (35-52)
frontend/javascripts/admin/rest_api.ts (1)
  • getSegmentSurfaceArea (893-918)
frontend/javascripts/viewer/model/scaleinfo.ts (1)
  • voxelToVolumeInUnit (10-24)
frontend/javascripts/libs/format_utils.ts (1)
  • formatNumberToArea (259-273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: frontend-tests
  • GitHub Check: build-smoketest-push
  • GitHub Check: backend-tests
🔇 Additional comments (7)
frontend/javascripts/viewer/view/context_menu.tsx (7)

1-7: LGTM! Imports are correct.

All new imports (BarChartOutlined, getSegmentSurfaceArea, formatNumberToArea, MenuInfo, getCurrentMappingName) are properly added and used within the file to support the new surface area statistics feature.

Also applies to: 25-30, 38-38, 88-88


200-205: LGTM! Context menu persistence is correctly implemented.

The modification correctly prevents the context menu from closing when the "load-stats" action is triggered, allowing users to see the statistics load without the menu disappearing.


1412-1412: LGTM! Condition change improves correctness.

The change from segmentIdAtPosition > 0 to segmentIdAtPosition !== 0 is correct. Segment ID 0 represents the background/no segment, so actions should be disabled for it. However, negative segment IDs (if they exist in the system) should not be automatically excluded. This aligns with the past review discussion.

Also applies to: 1431-1433


1666-1671: LGTM! Efficient on-demand loading pattern.

The state-based trigger mechanism ensures statistics are only fetched when explicitly requested by the user, reducing unnecessary API calls. Saving the tracing before fetching ensures the statistics reflect the latest state.


1804-1815: LGTM! Load statistics menu item is well-designed.

The menu item correctly:

  • Uses conditional rendering based on segment statistics availability
  • Provides a clear, dynamic label ("Load" vs "Reload")
  • Uses an appropriate icon (BarChartOutlined)
  • Prevents default event behavior and menu closing via the key check in hideContextMenu

1831-1831: LGTM! Icon styling is improved.

The rotation and margin adjustments to the PushpinOutlined icon improve visual presentation and consistency.

Also applies to: 1844-1845


1899-1938: LGTM! Statistics info rows are correctly implemented.

The three statistics (surface area, volume, bounding box) are rendered with:

  • Unique keys ("surfaceInfo", "volumeInfo", "boundingBoxPositionInfo") resolving the duplicate key issue from past reviews
  • Appropriate icons and formatting
  • Consistent copy-to-clipboard functionality
  • Correct conditional rendering based on statistics availability

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did re-testing: Still works 🎉

Should be good to go :D

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)

627-643: Fix the meshFileName guard and mappingType.

Two critical issues remain from previous reviews:

  1. Line 629: The comparison mappingNameForMeshFile.contains(request.body.meshFileName) will always be false. mappingNameForMeshFile has type Option[Option[String]] (from Fox.runOptional on line 625), but request.body.meshFileName is Option[String]. The .contains method checks for exact inner value equality, so this comparison never succeeds.

  2. Line 633: Hardcoding mappingType = "HDF5" is incorrect. Mesh files can be zarr3, neuroglancerPrecomputed, or other formats. According to FullMeshRequest, valid values are "json", "agglomerate", or "editableMapping", not file format identifiers. When loading from a precomputed mesh file, mappingType should likely be None since the mapping is embedded in the mesh file itself.

Apply this diff:

 surfaceAreas <- Fox.serialCombined(request.body.segmentIds) { segmentId =>
+  val useProvidedMesh = meshFileKeyOpt.isDefined &&
+    mappingNameForMeshFile.flatten == request.body.mappingName
   val fullMeshRequest = FullMeshRequest(
-    meshFileName =
-      if (mappingNameForMeshFile.contains(request.body.meshFileName)) request.body.meshFileName else None,
+    meshFileName = if (useProvidedMesh) request.body.meshFileName else None,
     lod = None,
     segmentId = segmentId,
     mappingName = request.body.mappingName,
-    mappingType = request.body.mappingName.map(_ => "HDF5"),
+    mappingType = None, // Mapping embedded in mesh file for precomputed meshes
     editableMappingTracingId = None,
     mag = Some(request.body.mag),
     seedPosition = None,
     additionalCoordinates = request.body.additionalCoordinates,
   )

The logic now:

  • Uses the provided mesh file only if a valid mesh file key was found AND the mesh file's mapping matches the requested mapping (or both are absent).
  • Sets mappingType to None for precomputed mesh files (ad-hoc meshing path will handle mappings differently).
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (1)

364-365: Clarify mappingType semantics.
Earlier concern about hardcoded "HDF5" was discussed and considered acceptable in this codebase. Add a brief comment to prevent future confusion about Zarr/HDF5 mapping types.

Example:

// "HDF5" also covers Zarr-backed mappings in our mesh pipeline
mappingType = baseMappingName.map(_ => "HDF5"),
🧹 Nitpick comments (3)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (3)

360-361: Respect meshFileName when available (avoid forcing ad‑hoc).
If the layer is not using an editable mapping and the client provides a meshFileName, pass it through to leverage static meshes. Falls back to ad‑hoc when editable mapping is active.

Apply this diff:

-              meshFileName = None, // Cannot use static meshfiles for dynamic volume layers
+              // Use static mesh files when possible; fall back to ad‑hoc for editable mappings
+              meshFileName = if (tracing.getHasEditableMapping) None else request.body.meshFileName,

365-366: Propagate editableMappingTracingId for editable mappings.
Make the intent explicit and avoid relying on downstream inference.

Apply this diff:

-              editableMappingTracingId = None,
+              editableMappingTracingId = volumeTracingService.editableMappingTracingId(tracing, tracingId),

351-377: Optional: add bounded parallelism or slow‑request logging.
Surface‑area per segment can be heavy. Consider a bounded-parallel traversal utility (if available) or wrap in logTime(slackNotificationService.noticeSlowRequest) for observability.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 89ed12c and c6aa9f2.

📒 Files selected for processing (4)
  • frontend/javascripts/admin/rest_api.ts (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (1 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala
  • frontend/javascripts/admin/rest_api.ts
🧰 Additional context used
🧬 Code graph analysis (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (6)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (2)
  • SegmentStatisticsParametersMeshBased (21-25)
  • SegmentStatisticsParametersMeshBased (26-29)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (4)
  • DSFullMeshService (38-245)
  • FullMeshRequest (22-32)
  • FullMeshRequest (34-36)
  • loadFor (56-65)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala (3)
  • MeshFileService (61-184)
  • lookUpMeshFileKey (70-75)
  • mappingNameForMeshFile (113-125)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DatasetCache.scala (1)
  • getWithLayer (30-36)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (8)
  • Fox (30-230)
  • Fox (232-305)
  • runOptional (159-169)
  • serialCombined (95-99)
  • serialCombined (99-111)
  • map (281-284)
  • map (377-377)
  • toFox (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)
  • surfaceAreaFromStlBytes (78-114)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (8)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/SegmentStatistics.scala (4)
  • SegmentStatisticsParameters (14-17)
  • SegmentStatisticsParameters (18-20)
  • SegmentStatisticsParametersMeshBased (21-25)
  • SegmentStatisticsParametersMeshBased (26-29)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala (3)
  • loadFor (56-65)
  • FullMeshRequest (22-32)
  • FullMeshRequest (34-36)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/TSFullMeshService.scala (1)
  • loadFor (40-50)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
  • getSegmentSurfaceArea (618-649)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteWebknossosClient.scala (1)
  • getAnnotationIdForTracing (91-102)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/annotation/TSAnnotationService.scala (2)
  • findVolume (678-692)
  • baseMappingName (642-651)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (7)
  • Fox (30-230)
  • Fox (232-305)
  • serialCombined (95-99)
  • serialCombined (99-111)
  • map (281-284)
  • map (377-377)
  • toFox (14-14)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/FullMeshHelper.scala (1)
  • surfaceAreaFromStlBytes (78-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: backend-tests
  • GitHub Check: frontend-tests
  • GitHub Check: build-smoketest-push
🔇 Additional comments (4)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (2)

27-32: LGTM!

The mesh-related imports are correctly added to support the new surface area endpoint.


77-77: LGTM!

The fullMeshService dependency injection is correctly added to support mesh loading in the new endpoint.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala (2)

16-18: Imports look correct for mesh-based stats.
No issues.


290-291: Good: unified error key.
Switch to "mesh.loadFull.failed" aligns with the new surface-area flow.

@fm3 fm3 merged commit ccf1c28 into master Oct 14, 2025
5 checks passed
@fm3 fm3 deleted the mesh-surface-area branch October 14, 2025 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Surface area for segments

4 participants